Add first draft of OEP-13 (API conventions) by efagin · Pull Request #36 · openedx/openedx-proposals

efagin · 2016-12-29T19:46:40Z

Supercedes https://openedx.atlassian.net/wiki/display/AC/edX+REST+API+Conventions (the content is virtually identical). A Best Practices OEP is a more reasonable home for this content and I'd like changes to our conventions to be captured more formally (i.e. through PRs instead of wiki comment threads).

@nasthagiri, would you mind acting as arbiter for this OEP? I can't imagine there are any issues but you're familiar enough with the original source wiki. Once this gets merged, I'm going to update the original wiki page to just link to the OEP.

andy-armstrong

@efagin converting this to an OEP was a great idea, thanks. I'm out of the loop on our current thinking around API development so I look forward to hearing how others react. I have a few minor comments that I hope are helpful.

andy-armstrong · 2016-12-29T22:16:21Z

oeps/oep-0013.rst

Nit: we settled on wrapping lines at 80 characters in OEPs.

From @cpennington:

I tend to prefer wrapped lines in the raw text, simply because it's easier to read in an editor (and in the email summaries that github sends). Github wraps them nicely in the diff, but in this case I lean towards optimizing towards the lowest common denominator.

IMO it would be helpful to have a description of the API gateway and how (if?) it affects the development of APIs.

Good point, I just updated the doc with 80-char line wrapping.

andy-armstrong · 2016-12-29T22:19:22Z

oeps/oep-0013.rst

Are we using HATEOAS in our newer APIs? I remember that @nasthagiri mentioned there were reasons why mobile doesn't use it. Is this still the recommendation? Does it change with the API gateway?

andy-armstrong · 2016-12-29T22:23:14Z

oeps/oep-0013.rst

Is this where we landed at the last arch lunch?

andy-armstrong · 2016-12-29T22:25:49Z

oeps/oep-0013.rst

Don't forget to provide this link

Thanks, done.

andy-armstrong · 2016-12-29T22:27:06Z

oeps/oep-0013.rst

Nit: this section shows up as quoted on the rendered page:

https://github.com/edx/open-edx-proposals/blob/6abc23aed18fe98f0270934363d744fc3ef2cdec/oeps/oep-0013.rst

I'd probably also bold the status codes themselves.

andy-armstrong · 2016-12-29T22:28:02Z

oeps/oep-0013.rst

Nit: missing a space between the key and value.

andy-armstrong · 2016-12-29T22:32:27Z

oeps/oep-0013.rst

I'm not sure if this document is still being updated, and it if it is then it is not the docs team that does it.

andy-armstrong · 2016-12-29T22:34:57Z

oeps/oep-0013.rst

Maybe pull this out to its own top level section? I thought there was a standard Future Directions in OEP-1 but I don't see one.

I removed it entirely, we should keep speculative things out of OEPs and just add content when it's well-formed.

andy-armstrong · 2016-12-29T22:36:14Z

oeps/oep-0013.rst

It might be useful to mention JWT tokens here.

andy-armstrong · 2016-12-29T22:38:18Z

oeps/oep-0013.rst

It would be good to update this to include some of the newer APIs, such as those for course discovery.

robrap · 2016-12-30T03:19:13Z

oeps/oep-0013.rst

I don't think we have implemented the xpathesque type of field filtering that Google has.

If we think we want to use this, for mobile or otherwise, we should choose a solution and duplicate and/or link to docs that provide details. For example, Google's docs (drive api) for partial responses.

robrap · 2016-12-30T03:34:33Z

oeps/oep-0013.rst

Some of our code seems to be using Django REST Swagger for Swagger documentation of DRF. Maybe this should be documented as our best practice, given that REST Framework Docs (which I think is documented below, was deprecated in favor of Django REST Swagger, both by the same creator. This is described on the DRF site for documenting APIs.

efagin · 2017-01-03T16:32:38Z

@andy-armstrong @robrap thanks for the early feedback! Many of your comments question the content itself. Since all I did was just port the existing wiki page over here, I'm surprised that there's so much in this document that you feel requires major editing. (This, on its own, is a good reason to move to OEP.)

Do you think we should broaden the scope of this OEP to actually go and revise these best practices? I had started this just figuring I'd do a quick port and merge, and then we could modify later, but if you think we should just do it all now, then I'll allocate more of my time to updating these best practices now.

andy-armstrong · 2017-01-03T18:23:48Z

@efagin IMO it would be great if you could allocate the extra time to revise the recommendations. It seems like a good opportunity to make this up-to-date, and then we can all be on the same page going forward.

efagin · 2017-01-03T19:19:37Z

@andy-armstrong OK, I'll do that. This OEP will definitely require more effort before it's ready to merge, and please continue to provide feedback.

andy-armstrong · 2017-01-03T22:27:35Z

oeps/oep-0013.rst

It would be good to describe how boolean query parameters should be handled. My preference is that they should be passed as true/false, but I'm open to other options.

I mention this because I dislike using 0/1 for boolean values, which I commented as such on this PR: https://github.com/edx/edx-platform/pull/14204

andy-armstrong · 2017-01-06T22:37:12Z

oeps/oep-0013.rst

I think we need to revisit our documentation approach for APIs given the changes in the doc team. I'm not sure if this OEP is the best place to describe processes, but it would be good to lay out what is expected of documentation for a new API:

the API developer should use Swagger to auto-generate documentation (as mentioned below)

the developer is in charge of getting this documentation published (but how?)

how does an API get to be part of the API Gateway and have its documentation made available there too?

Note that the link on this line is for edx-platform only, and is for a document that does not appear to be actively maintained. Also, this OEP needs to be for the community as a whole, so the language about "Work with the Docs team" doesn't generalize even if there were a docs team to work with.

clintonb · 2017-01-12T20:41:18Z

oeps/oep-0013.rst

Why zero-indexed? I can't think of any of our own APIs that implement this. If we want to see this, and the other conventions (e.g. num_pages), implemented we should make it easy to do so by creating a pagination class in https://github.com/edx/edx-drf-extensions.

clintonb · 2017-01-12T20:45:14Z

oeps/oep-0013.rst

Sorting/ordering should be separate from pagination.

I would prefer we advocate ordering on explicit fields as it is clear to the client what is being done. Passing sort_order=asc is quite ambiguous.

clintonb · 2017-01-12T20:47:35Z

oeps/oep-0013.rst

We use DRF 3 everywhere. This is a more relevant URL: http://www.django-rest-framework.org/topics/documenting-your-api/

clintonb · 2017-01-12T20:49:16Z

oeps/oep-0013.rst

This is not necessary when using Swagger. Also, docstrings like these tend to grow stale as APIs evolve since I don't necessarily have to touch a view to modify a serializer.

ormsbee · 2017-03-20T14:32:27Z

oeps/oep-0013.rst

Typo on "top-level"

ormsbee · 2017-03-20T14:47:09Z

oeps/oep-0013.rst

An example or two might be good here.

ormsbee · 2017-03-20T14:53:10Z

oeps/oep-0013.rst

Developers should explicitly set limits on this parameter.

ormsbee · 2017-03-20T14:56:56Z

oeps/oep-0013.rst

Can we add a note here about how certain very large collections (e.g. users, enrollments) cannot be effectively paged through with DRF's default pagination class? The choices then are to either use CursorPagination, a custom pagination class, or to simply cap the result set in some way:

"Returning the first 500 results that match your query."

"You have to be more specific before I give you any data."

"No, you may not crawl our entire enrollment database by making half a million sequential requests, please don't try that."

efagin · 2017-07-25T19:48:15Z

I know this has been idle for some time now. Part of the issue is the recurring theme that "best practice" OEPs are difficult to maintain and approve. For now, we've made the conscious decision at edX to keep Best Practice content in the wiki and allow freeform editing as practices evolve. Instead of leaving this open indefinitely, I'm going to withdraw this OEP, remove content, and merge the PR as-is. I will NOT squash commits unless this is desired - might be nice to have the commit history for archival purposes.

@andy-armstrong would you mind acting as Arbiter here?

andy-armstrong

👍 makes sense to me. As Arbiter, I'll go ahead and merge this now.

Add first draft of OEP-13 (API conventions)

6abc23a

efagin force-pushed the efagin/oep-13-api-conventions branch from 41871b6 to 6abc23a Compare December 29, 2016 19:54

andy-armstrong reviewed Dec 29, 2016

View reviewed changes

robrap reviewed Dec 30, 2016

View reviewed changes

fix a few points of feedback, and wrap lines where possible

85daad7

andy-armstrong reviewed Jan 3, 2017

View reviewed changes

andy-armstrong mentioned this pull request Jan 3, 2017

backend view to return tree for course outline and any given block. openedx/openedx-platform#14204

Merged

4 tasks

andy-armstrong reviewed Jan 6, 2017

View reviewed changes

clintonb reviewed Jan 12, 2017

View reviewed changes

ormsbee reviewed Mar 20, 2017

View reviewed changes

withdrawing OEP-13

f8c961f

andy-armstrong approved these changes Jul 25, 2017

View reviewed changes

andy-armstrong merged commit 4929541 into master Jul 25, 2017

andy-armstrong deleted the efagin/oep-13-api-conventions branch July 25, 2017 20:06

Conversation

efagin commented Dec 29, 2016

Uh oh!

andy-armstrong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efagin commented Jan 3, 2017

Uh oh!

andy-armstrong commented Jan 3, 2017

Uh oh!

efagin commented Jan 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efagin commented Jul 25, 2017

Uh oh!

andy-armstrong left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone